handle: use per-request sockets for VF-only configuration by l8huang · Pull Request #1209 · vishvananda/netlink

l8huang · 2026-08-06T08:44:14Z

description

PR #1174 changed ConfigureHandle to configure the package-wide pkgHandle through NewHandleWithOptions, which creates persistent netlink sockets in the pkgHandle.

Because pkgHandle.sockets remains bound to the network namespace where it was created, the following operations could target the wrong namespace after the calling OS thread changed namespaces.

This PR returns a handle without nil sockets when the caller only disables VF information collection:

NewHandleWithOptions(HandleOptions{
    DisableVFInfoCollection: true,
})

Summary by CodeRabbit

New Features
- Handles configured only to disable virtual-function information collection can now operate without a persistent socket.
- Network namespace behavior is now clearly documented for persistent and socketless handles.
Bug Fixes
- Preserved existing socket-based behavior when retry, namespace, or netlink family options are configured.
Tests
- Added coverage for option propagation, socket allocation, cleanup, retries, namespaces, and netlink family settings.

coderabbitai · 2026-08-06T08:44:36Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7d562717-25b0-4bed-9f54-da845a5be26b

📥 Commits

Reviewing files that changed from the base of the PR and between 156a440 and 37dcc4e.

📒 Files selected for processing (2)

handle_linux.go
handle_linux_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

handle_linux.go
handle_linux_test.go

📝 Walkthrough

Walkthrough

NewHandleWithOptions now creates a socketless handle when only DisableVFInfoCollection is enabled. Documentation clarifies network namespace behavior. Tests cover option propagation, socket allocation, request wiring, and cleanup.

Changes

Handle construction

Layer / File(s)	Summary
Handle option construction `handle_linux.go`	`NewHandleWithOptions` uses a socketless handle for the VF-info-only configuration. Other configurations retain persistent-socket construction. `HandleOptions.NetNS` documentation describes namespace behavior.
Handle construction validation `handle_linux_test.go`	Table-driven tests cover VF-info disabling, retries, network namespaces, netlink families, socket allocation, request socket wiring, option propagation, and cleanup.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

vishvananda/netlink#1174: Introduces NewHandleWithOptions and HandleOptions.
vishvananda/netlink#1207: Relates to NewHandleWithOptions and HandleOptions.RetryInterrupted testing.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: using per-request sockets for VF-only configuration.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

thediveo · 2026-08-06T19:52:58Z

I simply don't understand this PR and I'm afraid that because of its convoluted AI generated description that it would make the situation worse by complicating the current API hidden state and behavior even more.

l8huang · 2026-08-06T23:02:37Z

@thediveo updated PR description, hope it's easier to understand now

l8huang · 2026-08-06T23:29:56Z

+// specified, the returned Handle creates a socket for each request in the
+// caller's current network namespace instead of keeping persistent sockets.
 func NewHandleWithOptions(opts HandleOptions, nlFamilies ...int) (*Handle, error) {
+	if opts.DisableVFInfoCollection && !opts.RetryInterrupted && opts.NetNS == nil && len(nlFamilies) == 0 {


I don't know how widely is NewHandleWithOptions() used now, so just want to open a small window to allow disabling VFInfoCollection without init Handle.sockets.

PR vishvananda#1174 changed ConfigureHandle() to configure the package-wide pkgHandle through NewHandleWithOptions(), which creates persistent netlink sockets in the pkgHandle. Because pkgHandle.sockets remains bound to the network namespace where it was created, the following operations could target the wrong namespace after the calling OS thread changed namespaces. This PR returns a handle without nil sockets when the caller only disables VF information collection: NewHandleWithOptions(HandleOptions{ DisableVFInfoCollection: true, }) Signed-off-by: Lei Huang <leihuang.dev8@gmail.com>

coderabbitai · 2026-08-07T03:42:16Z

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

l8huang · 2026-08-07T15:48:25Z

@vishvananda @aboch could you please take a look? Thanks.

This was referenced Aug 6, 2026

node: disable netlink VF info collection ovn-kubernetes/ovn-kubernetes#6779

Open

Default zero value pkgHandle breaks v1 compatibility when discovering VFs #1204

Open

l8huang commented Aug 6, 2026

View reviewed changes

l8huang force-pushed the vf-only branch from 4394b22 to 37dcc4e Compare August 7, 2026 03:42

Navigation Menu

handle: use per-request sockets for VF-only configuration - #1209

handle: use per-request sockets for VF-only configuration#1209
l8huang wants to merge 1 commit into
vishvananda:mainfrom
l8huang:vf-only

l8huang commented Aug 6, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Aug 6, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

thediveo commented Aug 6, 2026 •

edited

Loading

Uh oh!

l8huang commented Aug 6, 2026

Uh oh!

l8huang Aug 6, 2026

Uh oh!

coderabbitai Bot commented Aug 7, 2026

Uh oh!

l8huang commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

l8huang commented Aug 6, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

description

Summary by CodeRabbit

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

thediveo commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

l8huang commented Aug 6, 2026

Uh oh!

l8huang Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot commented Aug 7, 2026

Uh oh!

l8huang commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

l8huang commented Aug 6, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 6, 2026 •

edited

Loading

thediveo commented Aug 6, 2026 •

edited

Loading